⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Differentiate stale report from concurrently deleted row.

At Line 98, RowsAffected == 0 unconditionally returns existing. That is correct for stale reports, but incorrect when the record was deleted between read and update. In that case this method reports success with a non-persisted object. Please branch explicitly: stale → return existing; missing row → re-read/recreate or return not-found.

Verify each finding against the current code and only fix it if needed.

In `@pkg/dao/adapter_status.go` around lines 98 - 100, When
updateResult.RowsAffected == 0 in the adapter status update flow, explicitly
distinguish a stale-report (concurrent version mismatch) from a row that was
deleted between read and update: after detecting RowsAffected == 0, re-query the
DB for the row using the same key/ID from existing (e.g., existing.ID) — if the
re-read returns no row, return a not-found error (or trigger recreate logic)
instead of returning existing; if the re-read returns a row whose version/fields
differ from existing, treat that as a stale update and return existing (or a
conflict error) so callers know the update didn’t persist. Ensure this logic
sits where updateResult.RowsAffected is checked (the function handling the
update in adapter_status.go).

The scenario CodeRabbit describes (record deleted between read and update) cannot happen in this code. The callers of Upsert in ProcessAdapterStatus hold a SELECT ... FOR UPDATE lock on the parent resource (cluster/nodepool) for the duration of the transaction. No concurrent transaction can delete the adapter status while this one is in progress.

@mliptak0, that's a valid point — the SELECT ... FOR UPDATE lock on the parent resource serializes all concurrent writes for the duration of the transaction, which makes the delete-between-read-and-update race impossible in this context. I've noted the locking strategy for future reviews.

📝 Learnings were identified and not saved (knowledge base disabled). Enable

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle zero-row status updates as not-found.

At Line 99, this returns success even when no row matches id (RowsAffected == 0). In hard-delete races, that silently drops a status write and hides state loss. Treat zero affected rows as not-found and roll back.

 func (d *sqlClusterDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error {
 	g2 := (*d.sessionFactory).New(ctx)
 	result := g2.Model(&api.Cluster{}).Where("id = ?", id).Update("status_conditions", statusConditions)
 	if result.Error != nil {
 		db.MarkForRollback(ctx, result.Error)
 		return result.Error
 	}
+	if result.RowsAffected == 0 {
+		err := gorm.ErrRecordNotFound
+		db.MarkForRollback(ctx, err)
+		return err
+	}
 	return nil
 }

Verify each finding against the current code and only fix it if needed.

In `@pkg/dao/cluster.go` around lines 97 - 105, In SaveStatusConditions
(sqlClusterDao.SaveStatusConditions) add a check after the Update to treat zero
affected rows as not-found: if result.RowsAffected == 0, call
db.MarkForRollback(ctx, gorm.ErrRecordNotFound) (or sql.ErrNoRows) and return an
appropriate not-found error instead of nil; keep the existing error handling for
result.Error unchanged so true DB errors still roll back.

False positive for the same reason as the Upsert comment.

In the ProcessAdapterStatus flow (the code path added by this PR), the cluster is locked with SELECT ... FOR UPDATE at the start of the transaction, preventing concurrent hard-delete. For the existing UpdateClusterStatusFromAdapters path (which doesn'thold a lock), silently dropping a status condition write for a concurrently hard-deleted cluster is harmless — the cluster no longer exists, so persisting its conditions serves no purpose.

@mliptak0, understood — that reasoning is sound. The SELECT ... FOR UPDATE lock in ProcessAdapterStatus eliminates the race in the new code path, and in UpdateClusterStatusFromAdapters a silent no-op on an already-hard-deleted cluster is semantically correct. I'll retract the comment.

📝 Learnings were identified and not saved (knowledge base disabled). Enable

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Propagate unexpected lookup failures before upserting.

This treats every FindByResourceAndAdapter error as “not found”. If that read fails for any other reason, the code still proceeds into Upsert, which hides the DAO failure and can take the wrong branch for freshness handling. Only normalize an actual miss to nil; return other errors immediately.

Verify each finding against the current code and only fix it if needed.

In `@pkg/services/adapter_status.go` around lines 125 - 130, The lookup code
around s.adapterStatusDao.FindByResourceAndAdapter currently treats any error as
a "not found" by setting existing = nil, which can hide real DAO failures before
calling Upsert; update the logic in the function to only normalize to nil when
the DAO returns the explicit not-found sentinel (e.g., ErrNotFound or the dao's
not-found behavior) and to return the lookup error immediately for any other
error; locate the call to s.adapterStatusDao.FindByResourceAndAdapter(ctx,
adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter) and
replace the unconditional existing = nil on error with a conditional that checks
the error value/type and either sets existing = nil for not-found or returns the
error so Upsert sees only valid existing state.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require Finalized=True at the current generation.

allAdaptersFinalized only checks IsFinalized(). After soft-delete bumps the resource generation, an older Finalized=True row still counts here, so cluster/nodepool hard-delete can run before every required adapter has finalized the delete generation. This helper should filter on the current generation, the same way computeReconciled does.

-func allAdaptersFinalized(requiredAdapters []string, adapterStatuses api.AdapterStatusList) bool {
+func allAdaptersFinalized(
+	requiredAdapters []string,
+	currentGeneration int32,
+	adapterStatuses api.AdapterStatusList,
+) bool {
 	finalizedAdapters := make(map[string]struct{})

 	for _, adapterStatus := range adapterStatuses {
-		if adapterStatus.IsFinalized() {
+		if adapterStatus.ObservedGeneration == currentGeneration && adapterStatus.IsFinalized() {
 			finalizedAdapters[adapterStatus.Adapter] = struct{}{}
 		}
 	}

Verify each finding against the current code and only fix it if needed.

In `@pkg/services/aggregation.go` around lines 740 - 754, The helper
allAdaptersFinalized currently only checks adapterStatus.IsFinalized() and thus
considers finalization from any generation; change it to require Finalized==true
at the current resource generation by adding a generation parameter (e.g.,
currentGeneration int64) and only count adapterStatus entries whose Generation
equals that parameter and whose IsFinalized() is true; update callers
(cluster/nodepool hard-delete call sites) to pass the current resource
generation, mirroring the generation filtering approach used by
computeReconciled, so older finalization rows are ignored.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Re-check the owning cluster after hard-deleting the last node pool.

Once tryHardDeleteNodePool succeeds, this flow returns immediately. If the parent cluster was already soft-deleted and its adapters had finalized earlier, nothing reevaluates that cluster when the final child disappears, so it can stay stranded until another cluster status update happens. The new cluster integration test currently works around this by reposting a cluster adapter after nodepool deletion. Use nodePool.OwnerID to trigger the parent cluster’s hard-delete/status recomputation here.

Also applies to: 381-403

Verify each finding against the current code and only fix it if needed.

In `@pkg/services/node_pool.go` around lines 272 - 279, After a successful hard
delete in the node-pool deletion path (when nodePool.DeletedTime != nil and
tryHardDeleteNodePool returns true), ensure we re-check/requeue the parent
cluster using nodePool.OwnerID so the cluster's hard-delete/status is recomputed
now that the final child is gone; update the same logic in the other occurrence
(around lines 381-403) to also trigger the parent cluster reconciliation instead
of returning immediately (use the existing cluster-reconciliation/enqueue helper
on the service, e.g., the method responsible for cluster status recomputation or
enqueuing by ID).

The gap is real: after the last nodepool is hard-deleted, nothing automatically triggers the cluster's hard-delete check. The integration test though covers this by manually re-posting a cluster adapter status.

However, fixing this in this PR is a heavy lift because NodePoolService doesn't have a reference to ClusterService — adding one creates a cross-service dependency and potential circular dependency. The current design is event-driven: Sentinel periodically triggers adapter status updates, so the cluster will eventually get re-evaluated. Probably good case to be covered by e2e tests...

@mliptak0, that makes sense — the circular-dependency concern is a real constraint, and the Sentinel-driven eventual consistency model is a reasonable trade-off here. The integration test's manual re-post approach is a fair workaround for now.

Would you like me to open a follow-up GitHub issue to track the eventual-consistency gap (cluster hard-delete not triggered automatically after last node pool is removed) so it's not lost? It could also serve as a reminder to add E2E coverage for that scenario.

It could be simplified using raw SQL: SELECT EXISTS(SELECT 1 FROM node_pools WHERE owner_id = ?)

This isn't part of this PR, but since we have a Save() method which updates from the model, would it be better to use GetForUpdate here?

Same here, maybe put GetForUpdate is better?